-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dividers and fix bug in siteNav #1063
Conversation
* 'master' of https://github.com/MarkBind/markbind: Update tests Allow using 'none' footer attribute in frontmatter (MarkBind#1002) Support line numbers for code blocks (MarkBind#991) 2.11.0 Update test files due to changes in PR MarkBind#982 Update vue-strap version to v2.0.1-markbind.36 Make highlighting bold (MarkBind#1045) Support markdown for header attr in dropdown (MarkBind#1029) Add '_site' to the ignored folders in site.json (MarkBind#1046) Use path.join instead of string interpolation (MarkBind#1052) Implement box markdown header attributes parsing (MarkBind#1025) Make the position of top navbar fixed (MarkBind#982) Exclude *.md files from being copied over on build (MarkBind#1010) # Conflicts: # docs/css/main.css
Fixes #1064 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing the mistake.
Just a nit, I think it would be better to use MarkBind's code block syntax since we have one now.
As per discussion in #543. I'm not sure whether we should include horizontal dividers in the example code, or show the example code without horizontal dividers and then mention that you can divide via horizontal rule. I think the former is fine, we just have to mention that feature at the 'Step to add a siteNav'. The additional change in bold below.
Steps to add a siteNav:
- Format your siteNav as an unordered Markdown list and save it inside the
_markbind/navigation
directory. You may include dividers in the siteNav via horizontal rules.- Specify the file as the value of the
siteNav
attribute in the<frontmatter>
of the page.
<code> | ||
* [:house: Home]({{ showBaseUrlText }}/index.html)<br> | ||
* Docs<br> | ||
--- | ||
<br>* Docs<br> | ||
* [User Guide]({{ showBaseUrlText }}/ug.html)<br> | ||
* [Dev Guide]({{ showBaseUrlText }}/dg.html)<br> | ||
* [Search]({{ showBaseUrlText }}/search.html)<br> | ||
--- | ||
<br>* [Search]({{ showBaseUrlText }}/search.html)<br> | ||
* [Google Search](https://www.google.com/)<br> | ||
* [YouTube Search](https://www.youtube.com/)<br> | ||
* [Contact]({{ showBaseUrlText }}/contact.html) | ||
--- | ||
<br>* [Contact]({{ showBaseUrlText }}/contact.html) | ||
</code> | ||
</box> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MarkBind now supports code blocks.
```{.no-line-numbers}
* [:house: Home]({{ baseUrl }}/index.html)
* Docs
* [User Guide]({{ baseUrl }}/ug.html)
* [Dev Guide]({{ baseUrl }}/dg.html)
* [Search]({{ baseUrl }}/search.html)
* [Google Search](https://www.google.com/)
* [YouTube Search](https://www.youtube.com/)
* [Contact]({{ baseUrl }}/contact.html)
```
Could be used instead. Much easier syntax for documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want me to update it to using code blocks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice. You could also change the other code blocks in this file into MarkBind code blocks, but that would expand the scope of your ticket by abit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. Let me just change this to code block in this PR. Since this PR already affects this. For rest, I will do another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Regarding that other PR, it would be best to wait after #1034 is merged. After that merge, the code blocks will have (almost) full functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking on Contact shows Page Not Found. Any suggestion as to what to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbriannl can I get your review on this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, just one more change then we're good.
Co-Authored-By: nbriannl <neilbrian.nl@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@marvinchin Can I get your review on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions!
@@ -3,26 +3,23 @@ | |||
**A _Site Navigation Menu_ (==_siteNav_ for short==) is a fixed menu on the ==left edge== of a page**, that can be used to show a road map of the main pages of your site. | |||
|
|||
Steps to add a siteNav: | |||
1. Format your siteNav as an unordered Markdown list and save it inside the `_markbind/navigation` directory. | |||
1. Format your siteNav as an unordered Markdown list and save it inside the `_markbind/navigation` directory. You may include dividers in the siteNav via <a target="_blank" href="{{ baseUrl }}/userGuide/readerfacingfeatures#horizontal-rules">horizontal rules</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we shouldn't open internal links in a new tab. I know that we currently do that in the page and site nav pages, but perhaps we should remove those too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want me to open in same page or remove link altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open it in the same page.
* [Contact]({{ showBaseUrlText }}/contact.html) | ||
</code> | ||
</box> | ||
```{.no-line-numbers} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think adding a divider in the example would be helpful, since we mentioned it above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should. So that they can understand how it looks and accordingly decide whether to use or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my bad. I was asking you whether to do it or not. Didn't realise you were saying to implement and show. I have done it now. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry for the lack of clarity!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The horizontal rules look good. Just one nit!
* Docs | ||
* [User Guide]({{ baseUrl }}/ug.html) | ||
* [Dev Guide]({{ baseUrl }}/dg.html) | ||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a nit, but let's not leave trailing spaces behind 🙂
You should also be able to configure your editor to automatically remove trailing whitespace behind, so you don't have to worry about it yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…nvert-to-code-block * 'master' of https://github.com/MarkBind/markbind: Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062) # Conflicts: # docs/userGuide/syntax/siteNavigationMenus.mbdf
* 'master' of https://github.com/MarkBind/markbind: 2.12.0 Update outdated test files Update vue-strap version to v2.0.1-markbind.37 Fix refactor to processDynamicResources (MarkBind#1092) Implement lazy page building for markbind serve (MarkBind#1038) Add warnings for conflicting/deprecated component attribs (MarkBind#1057) Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062)
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Documentation update
• [X] Bug fix
What is the rationale for this request?
Add dividers to siteNav
What changes did you make? (Give an overview)
Added dividers to siteNav and fix bug of input/output mismatch.
Provide some example code that this change will affect:
Is there anything you'd like reviewers to focus on?
N.A.
Testing instructions:
Deployed preview